Post-merge-review: use scope manager for block-param tracking in template-no-obsolete-elements#2676
Conversation
The rule suppressed obsolete-element reports when the tag name shadows a
block param from a GlimmerBlockStatement (e.g. {{#let (...) as |marquee|}}).
It did not track element-level block params (<Comp as |marquee|>), so
inside an angle-bracket component the shadowing was ignored and the tag
was falsely reported. Push element block params on enter and pop on exit,
mirroring the existing GlimmerBlockStatement handling.
johanrd
force-pushed
the
night_fix/template-no-obsolete-elements
branch
from
511b615 to
029c2c0
Compare
| // Element-level block params (e.g. `<Comp as |param|>`) are scoped to | ||
| // the children, so push them after the obsolete check. Pop on exit. | ||
| const elementParams = node.blockParams || []; | ||
| blockParamsInScope.push(...elementParams); |
There was a problem hiding this comment.
shouldn't this be the responsibility of the scope manager?
There was a problem hiding this comment.
added a clarification above blockParamsInScope now.
There was a problem hiding this comment.
kinda seems like we should fix that rather than add a work-around here, ya?
There was a problem hiding this comment.
yes, maybe, although I'm not sure how to approach that.
To my understanding, getScope() runs into two problems:
HBS mode: ember-eslint-parser's HBS path creates an intentionally empty scope manager — Glimmer block params from {{#let ... as |x|}} and <Comp as |x|> are not registered, so getScope() sees nothing.
GTS mode: at the GlimmerElementNode visitor, the element's own as |x| params are already visible in scope. So getScope() at <marquee as |marquee|> test would find marquee in scope and incorrectly suppress the flag. The push-after-check ordering handles this: the element is checked against outer params only, then its own are pushed for its children.
or were you thinking something else completely? upstream to ember-eslint-parser? thanks.
There was a problem hiding this comment.
Switched to scope-manager (getScope(node.parent) + chain walk) as suggested. 36/37 tests pass; the one failing HBS test is blocked on ember-tooling/ember-eslint-parser#189
…params Replaces the manual block-param stack with sourceCode.getScope(node.parent) + a walk up the scope chain. GTS templates work today (36/37 tests pass). BLOCKED BY UPSTREAM: ember-tooling/ember-eslint-parser#189. The HBS parser currently builds an empty scope manager, so Glimmer block params aren't registered for .hbs files. The failing test `{{#let ... as |plaintext|}}<plaintext />` documents this gap and will start passing once PR 189 is merged and consumed here. Uses getScope(node.parent) rather than getScope(node) so an element's own `as |x|` params (attached to that element's block scope) don't shadow its own tag — e.g. `<marquee as |marquee|>` must still flag the outer <marquee>.
johanrd
force-pushed
the
night_fix/template-no-obsolete-elements
branch
from
3a7cb52 to
6189818
Compare
johanrd
changed the title
template-no-obsolete-elements: track element-level block params2 participants
Summary
Replaces the manual block-param stack with
sourceCode.getScope(node.parent)+ a walk up the scope chain. Uses the parent scope rather than the element's own scope so an element's ownas |x|params don't shadow its own tag — e.g.<marquee as |marquee|>must still flag the outer<marquee>.Blocked by upstream
ember-tooling/ember-eslint-parser#189. The HBS parser currently builds an empty scope manager, so Glimmer block params aren't registered for
.hbsfiles. The failing test{{#let ... as |plaintext|}}<plaintext />in HBS mode documents this gap and will start passing once that PR is merged and consumed here.GTS templates work today because the gjs/gts parser already registers block params.
Test plan
<plaintext>/<marquee>flagged{{#let ... as |plaintext|}}<plaintext />not flagged (scope sees the block param)<Comp as |plaintext|><plaintext />not flagged (element-level block params)<marquee as |marquee|>still flagged (parent scope trick){{#let ... as |plaintext|}}<plaintext />not flagged — blocked on upstream PR 189<marquee>,<acronym>, etc. flagged